Hello @TomAugspurger! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 07, 2019 at 15:39 Hours UTC

Codecov Report

Merging #24279 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24279      +/-   ##
==========================================
+ Coverage   92.22%   92.22%   +<.01%     
==========================================
  Files         162      162              
  Lines       51828    51830       +2     
==========================================
+ Hits        47798    47801       +3     
+ Misses       4030     4029       -1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040f06f...d96942b. Read the comment docs.

Codecov Report

Merging #24279 into master will increase coverage by 1.62%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24279      +/-   ##
==========================================
+ Coverage   92.22%   93.84%   +1.62%     
==========================================
  Files         162      166       +4     
  Lines       51828    70087   +18259     
==========================================
+ Hits        47798    65774   +17976     
- Misses       4030     4313     +283

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040f06f...d96942b. Read the comment docs.

can you do the same for Index

The same what? This is just changing the base ExtensionArray repr.

The same what? This is just changing the base ExtensionArray repr.

yes, the same, to avoid departing from what we do in Index, which is pretty similar.

Sorry, I'm still not following what you're suggesting.

https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexes/base.py#L929

this is called _format_attrs in Index, so change one or the other

IIUC, Index._format_attrs returns a List[Tuple[str, str]]. So things like [('name', self.name), ('dtype', self.dtype)]

ExtensionArray._repr_extra doesn't require key-value pairs. Rather, we just expect a bit of text to include. I don't think it would be appropriate to require key-value pairs here.

IIUC, Index._format_attrs returns a List[Tuple[str, str]]. So things like [('name', self.name), ('dtype', self.dtype)]

ExtensionArray._repr_extra doesn't require key-value pairs. Rather, we just expect a bit of text to include. I don't think it would be appropriate to require key-value pairs here

This is just yet another difference with index. I would rather not have these differences.

I could imagine a subclass wanting to include something that's not a key-value pair.

This isn't a priority for me right now. I'd say merge if we think it's an improvement, or close and do it later. Either works for me.

I guess this is fine. ideally just have some integration with Index here (basically use the _repr_extra_ there as well for consistency.

Yes, DatetimeArray should use freq for it.

I don't know which _attributes attribute you're referring to. I don't think there's anything like that on ExtensionArray. DatetimeArray defines _attributes = ['freq', 'tz'], but the tz is already encoded in the dtype.

I don't know which _attributes attribute you're referring to. I don't think there's anything like that on ExtensionArray. DatetimeArray defines _attributes = ['freq', 'tz'], but the tz is already encoded in the dtype.

Ah, yes, I meant that _attributes, but assumed it only had freq in it (exactly because tz is already in the dtype), but didn't actually check ..

looking again. this is reasonable. can you merge master. are there other places that you see this being used in the current EA's? (other than Decimal).

closing this; I think we need more consolidation on repr to avoid code duplication.